BAF-1256 # Module Gateway - QUIC Integration #49
Conversation
…pport to ExternalClient
…ogging, and implement connection management.
…lement connection state management, improve logging, and refine cleanup processes.
…improved state debugging
…rectional communication
…ove outdated code sections, and organize code structure for clarity.
…ogging for stream events, and clean up debug code.
…ror handling with `StreamShutdown`, and clean up unused callbacks and logging.
…mprove stream and connection logging, and clean up unnecessary debug code.
…hod, improve handling of JSON-encoded protocol settings, and refine `SettingsParser` logic for cleaner value parsing.
…ne logging messages for clarity, and remove unused includes.
…better error handling, improve sender loop logic, and refine logging for message and connection events.
…ate with external_client::connection::ConnectionState
…ings::Constants::ALPN` for improved configurability.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…s after `receiveMessage` calls.
…sistent include order and readability.
…ed memory handling, update QUIC stream send logic, refine logging with stream IDs, and switch `initRegistration` to use `std::string`.
…er::logError` for error handling and ensure graceful returns in failure scenarios.
…::unique_ptr` with `std::string` for memory management.
…proper setup before logging.
…oped_lock` and simplify enum string conversion using `using enum`.
| ZLIB::ZLIB | ||
| fleet-protocol-cxx-helpers-static::fleet-protocol-cxx-helpers-static | ||
| async-function-execution-shared::async-function-execution-shared | ||
| msquic |
There was a problem hiding this comment.
The msquic shall be added as a dependency to the Fleet Protocol Context
https://github.com/bringauto/packager-fleet-protocol-context
@koudis will add it in week 19. 1. 2026
| case CONNECTING: return "Connecting"; | ||
| case CONNECTED: return "Connected"; | ||
| case CLOSING: return "Closing"; | ||
| default: return "Unknown"; |
There was a problem hiding this comment.
Use string constants like loggerVerbosityToString.
Put each return statements on a new line, like in loggerVerbosityToString.
| QUIC_BUFFER buffer{}; | ||
| std::string storage; | ||
|
|
||
| explicit SendBuffer(size_t size) |
There was a problem hiding this comment.
Maybe add a small docstring for struct SendBuffer, but definitely add a docstring for the constructor saying, that it initializes the buffer to size and fills it with zeroes
There was a problem hiding this comment.
update the README in resources/config with descriptions of the new options you added
| { | ||
| auto copy = std::make_shared<ExternalProtocol::ExternalClient>(*message); | ||
| std::scoped_lock lock(outboundMutex_); | ||
| outboundQueue_.push(std::move(copy)); |
There was a problem hiding this comment.
use unique_ptr - std::move(outboundQueue_.front()) and then outboundQueue_.pop() should work fine
also just a nitpick, but if you're only locking a single mutex, use lock_guard instead of scoped_lock
| alpnBuffer_.Length = static_cast<uint32_t>(alpn_.size()); | ||
|
|
||
| loadMsQuic(); | ||
| initRegistration("module-gateway-quic-client"); |
There was a problem hiding this comment.
"module-gateway-quic-client" should be defined as a constexpr string_view, also if initRegistration is only going to be used like this, and the appName will not be configurable, then initRegistration should just have no arguments and use this constant directly
| } | ||
|
|
||
| void QuicCommunication::initConfiguration() { | ||
| configurationOpen(nullptr); |
There was a problem hiding this comment.
I think you should parse the config for QUIC_SETTINGS as well, the JSON keys could just be the same as the member names in https://github.com/microsoft/msquic/blob/main/docs/api/QUIC_SETTINGS.md, should probably be done in a new static class, responsible for QUIC config parsing, since there's a lot of options - you will need something slightly different from getProtocolSettingsString, each of the QUIC_SETTINGS should be optional, so it can't throw when it doesn't find the key, also you will need to parse the value as an unsigned integer, not just a string
we probably don't need all the settings, probably just allow parsing of timeout settings for now
just make it simple to add any additional options we might need in the future
| event->RECEIVE.BufferCount | ||
| ); | ||
|
|
||
| std::vector<uint8_t> data; |
There was a problem hiding this comment.
according to https://github.com/microsoft/msquic/blob/main/docs/Streams.md#Receiving, BufferCount == 0 is valid and means end of stream data, maybe handle it as well?
There was a problem hiding this comment.
Resolved in d2303a7
This is not treated as stream termination; data processing is skipped only.
…initRegistration` with a constant for simplification and improved consistency.
…ique_ptr` in outbound queue, update `sendViaQuicStream` to use references for improved memory management and clarity.
…pand protocol support, and provide examples for both.
…dBuffer` to clarify purpose, usage, and construction.
… `settings::Constants` for improved maintainability.
…uicCommunication`.
|
Implementation of Quic external client connection